-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
🌱 Adding certwatcher as an external package #1441
🌱 Adding certwatcher as an external package #1441
Conversation
pkg/certwatcher/certwatcher_test.go
Outdated
|
||
func publicKey(priv interface{}) interface{} { | ||
switch k := priv.(type) { | ||
case *rsa.PrivateKey: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we properly type the function signature if this effectively has to be a *rsa.PrivateKey
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I'll be honest cert gen was a bit of copy/pasta where they had the ability to create multiple algo based certs ecdsa/rsa etc and I just trimmed this down. Given this is only one algo I can just hard code this type on line https://github.com/kubernetes-sigs/controller-runtime/pull/1441/files/efd29024698fb095a8c004dea2641925655fb4ad#diff-cc4e056d68e41389001be7de46dfac27f508da1039369db677584b5721321ee4R162
derBytes, err := x509.CreateCertificate(rand.Reader, &template, &template, publicKey(priv), priv)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
efd2902
to
d1bb67c
Compare
thanks for the review @alvaroaleman, both comments have been addressed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes LGTM, @christopherhein could we add a doc.go
in this package to explain what certwatcher
should/can be used for?
Definitely! 👍 |
d1bb67c
to
032f250
Compare
Signed-off-by: Chris Hein <me@chrishein.com>
032f250
to
a93a723
Compare
@vincepri I did ya one a little better, added a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/approve
/assign @alvaroaleman
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alvaroaleman, christopherhein, vincepri The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Per #1292 this exports the
certwatcher
package for use with other packages, for example customRunnable
's passed into a manager.Closes #1292
Signed-off-by: Chris Hein me@chrishein.com